Close RubberDuck Issues #171
Conversation
Replace generic functions with typed functions `$ for String` Use VBNullString instead of "" json_ParseErrorMessage returns a string Pass parameters ByVal unless assigned a value ByRef Always use Long instead of Integer. This prevents overflows on 64-bit systems and is better handled by modern CPUs.
|
About the
I would prefer the way it still is as its less code to read. If RubberDuck keeps bugging you with this, write ByRef where it is left out, fe |
|
Thanks for citing the documentation. You are correct, @jonadv. However in practice I've never seen a performance impact. I had this question myself when I first started using RubberDuck. The answer I got from Mathieu Guindon (the author and MS MVP for VBA) is that your code should say what it does. When you say something is To keep the default The RubberDuck documentation for this inspection is here: https://rubberduckvba.com/Inspections/Details/ParameterCanBeByVal |
|
I agree with @A9G-Data-Droid; I strongly prefer explicitly stating which is which. There are many cases where I thought I was doing it one way (especially with inherited code), but the routines were acting on the passed in values instead of the output. Explicitly stating which is which protects you from yourself, and protects your progeny from mishaps when they're on boarding to your project. While it might be the "right" way otherwise, I don't think any measurable performance impact outweighs the need. |
| utc_wYear As Long | ||
| utc_wMonth As Long | ||
| utc_wDayOfWeek As Long | ||
| utc_wDay As Long | ||
| utc_wHour As Long | ||
| utc_wMinute As Long | ||
| utc_wSecond As Long | ||
| utc_wMilliseconds As Long | ||
| End Type | ||
|
|
||
| Private Type utc_TIME_ZONE_INFORMATION | ||
| utc_Bias As Long | ||
| utc_StandardName(0 To 31) As Integer | ||
| utc_StandardName(0 To 31) As Long | ||
| utc_StandardDate As utc_SYSTEMTIME | ||
| utc_StandardBias As Long | ||
| utc_DaylightName(0 To 31) As Integer | ||
| utc_DaylightName(0 To 31) As Long |
There was a problem hiding this comment.
These must stay as Integer types; changing them to Long will break them and they will not contain the correct data.
There was a problem hiding this comment.
https://docs.microsoft.com/en-us/windows/win32/api/minwinbase/ns-minwinbase-systemtime for reference; the Word type is two bytes wide, and will result in incorrect handling otherwise.
|
Anybody who cares to try out the performance impact or suggested changes with my self written benchmark tool? 😁 |
I used The RubberDuck VBA code inspection to perform a code review. I closed out all issues that made sense as of this writing. There should be no functional change in operation.
Replace generic functions with typed functions
$ for StringUse VBNullString instead of ""
json_ParseErrorMessage returns a string
Pass parameters ByVal unless assigned a value ByRef
Always use Long instead of Integer. This prevents overflows on 64-bit systems and is better handled by modern CPUs.